Fix self profiler ICE on Windows #56170
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
wesleywiser
force-pushed
the
fix_self_profiler_windows
branch
from
290cbbc
to
13c81da
Compare
| if self.opts.debugging_opts.self_profile { | ||
| let mut profiler = self.self_profiling.borrow_mut(); | ||
| f(&mut profiler); | ||
| } |
There was a problem hiding this comment.
Was this change needed? (Not that it's wrong, just surprised it wasn't already the case).
There was a problem hiding this comment.
Not needed per se. It wasn't already the case because we showed a very small amount of overhead from turning on the profiler. I think it's probably best to make it opt-in though.
|
❤️ ❤️ ❤️ |
On Windows, the high-resolution timestamp api doesn't seem to always be monotonic. This can cause panics when the self-profiler uses the `Instant` api to find elapsed time. Work around this by detecting the case where now is less than the start time and just use 0 elapsed ticks as the measurement. Fixes rust-lang#51648
wesleywiser
force-pushed
the
fix_self_profiler_windows
branch
from
13c81da
to
dce1c45
Compare
|
@bors r=estebank |
|
📌 Commit dce1c45 has been approved by |
bors
added
S-waiting-on-bors
…ws, r=estebank Fix self profiler ICE on Windows Fixes rust-lang#51648
Rollup of 14 pull requests Successful merges: - #56024 (Don't auto-inline const functions) - #56045 (Check arg/ret sizedness at ExprKind::Path) - #56072 (Stabilize macro_literal_matcher) - #56075 (Encode a custom "producers" section in wasm files) - #56100 (generator fields are not necessarily initialized) - #56101 (Incorporate `dyn` into more comments and docs.) - #56144 (Fix BTreeSet and BTreeMap gdb pretty-printers) - #56151 (Move a flaky process test out of libstd) - #56170 (Fix self profiler ICE on Windows) - #56176 (Panic setup msg) - #56204 (Suggest correct enum variant on typo) - #56207 (Stabilize the int_to_from_bytes feature) - #56210 (read_c_str should call the AllocationExtra hooks) - #56211 ([master] Forward-ports from beta) Failed merges: r? @ghost
There was a problem hiding this comment.
I think there might be an error with how this workaround was applied. This is my first time delving in to rustc though so I could be way off the mark, sorry if that's the case.
Also, I just remembered: I think this issue is present on many more platforms than just windows and would be better handled by modifying the elapsed function in libstd or wherever it is, as well as editing the documentation and remove the statement that guarantees them to be nondecreasing.
| if self.current_timer >= now { | ||
| Duration::new(0, 0) | ||
| } else { | ||
| self.current_timer.elapsed() |
There was a problem hiding this comment.
Shouldn't this line (209) be changed to do now - self.current_timer instead? I think right now it's susceptible to the same error the workaround is meant to prevent. Since Instant::elapsed will call Instant::now again, which could be inaccurate and won't be double-checked like the variable now just was.
There was a problem hiding this comment.
That seems like a reasonable change to me
|
@IntrepidPig The real work around here for all platforms not just Windows was the first commit: b319715 Disable the self-profiler unless the None of the bug reports we've received so far have indicated that the user was actually trying to run the profiler, the were just trying to compile some code. |
Fixes #51648